Skip to content

Fix StringSet tests on portable runners #31999

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Abacn merged 4 commits into from
Jul 31, 2024
Merged

Fix StringSet tests on portable runners #31999

Abacn merged 4 commits into from
Jul 31, 2024

Conversation

@Abacn
Copy link
Contributor

@Abacn Abacn commented Jul 26, 2024

Reverts #31818

@Abacn
Copy link
Contributor Author

Abacn commented Jul 26, 2024

after #31838 now portable runners can see stringset metrics, however, the result is different from non-portable ones and still failing the assert

Previous:

java.lang.AssertionError: 
Expected: iterable with items [
     MetricResult{inNamespace="org.apache.beam.sdk.metrics.MetricsTest", name="sources", step="MyStep1", attempted=<StringSetResult{stringSet=[gcs]}>},
     MetricResult{inNamespace="org.apache.beam.sdk.metrics.MetricsTest", name="sinks", step="MyStep2", attempted=<StringSetResult{stringSet=[kafka, bq]}>},
     MetricResult{inNamespace="org.apache.beam.sdk.metrics.MetricsTest", name="sideinputs", step="MyStep1", attempted=<StringSetResult{stringSet=[bigtable, spanner]}>},
     MetricResult{inNamespace="org.apache.beam.sdk.metrics.MetricsTest", name="sideinputs", step="MyStep2", attempted=<StringSetResult{stringSet=[sql, bigtable]}>}
] in any order
     but: no item matches: ... in []

now:

but: not matched: <
MetricResult{key=MyStep1-ParMultiDo-Anonymous-:org.apache.beam.sdk.metrics.MetricsTest:sources, committedOrNull=null, attempted=StringSetResult{stringSet=[gcs]}}>

checked that metrics.getStringSets() now returns

[
MetricResult{key=MyStep2:org.apache.beam.sdk.metrics.MetricsTest:sinks, committedOrNull=null, attempted=StringSetResult{stringSet=[kafka, bq]}},
MetricResult{key=MyStep1-ParMultiDo-Anonymous-:org.apache.beam.sdk.metrics.MetricsTest:sources, committedOrNull=null, attempted=StringSetResult{stringSet=[gcs]}},
MetricResult{key=MyStep2:org.apache.beam.sdk.metrics.MetricsTest:sideinputs, committedOrNull=null, attempted=StringSetResult{stringSet=[bigtable, sql]}},
MetricResult{key=MyStep1-ParMultiDo-Anonymous-:org.apache.beam.sdk.metrics.MetricsTest:sideinputs, committedOrNull=null, attempted=StringSetResult{stringSet=[spanner, bigtable]}},
MetricResult{key=MyStep2:org.apache.beam.sdk.metrics.MetricsTest:sinks, committedOrNull=null, attempted=StringSetResult{stringSet=[kafka, bq]}},
MetricResult{key=MyStep1-ParMultiDo-Anonymous-:org.apache.beam.sdk.metrics.MetricsTest:sources, committedOrNull=null, attempted=StringSetResult{stringSet=[gcs]}},
MetricResult{key=MyStep2:org.apache.beam.sdk.metrics.MetricsTest:sideinputs, committedOrNull=null, attempted=StringSetResult{stringSet=[bigtable, sql]}},
MetricResult{key=MyStep1-ParMultiDo-Anonymous-:org.apache.beam.sdk.metrics.MetricsTest:sideinputs, committedOrNull=null, attempted=StringSetResult{stringSet=[spanner, bigtable]}}
]

the problem is each result duplicated one more time

@Abacn Abacn mentioned this pull request Jul 26, 2024
Closed
17 tasks
@github-actions github-actions bot added the java label Jul 26, 2024
@Abacn
Copy link
Contributor Author

Abacn commented Jul 26, 2024

Java PVR Spark Batch two tests failed: testValueStateCoderInferenceFromInputCoder testValueStateCoderInference unrelated to the change

Java ULR stringset test failed, still need to exclude it

@Abacn
Copy link
Contributor Author

Abacn commented Jul 26, 2024 •
edited
Loading

Dataflow v2 test Still failing:

org.apache.beam.sdk.metrics.MetricsTest$AttemptedMetricTests > testAttemptedStringSetMetrics FAILED
    java.lang.AssertionError at MetricsTest.java:417
org.apache.beam.sdk.metrics.MetricsTest$CommittedMetricTests > testCommittedStringSetMetrics FAILED
    java.lang.AssertionError at MetricsTest.java:288

will investigate further

Other portable runner tests (Flink/Spark) now passing

@Abacn Abacn marked this pull request as ready for review July 26, 2024 20:48
@Abacn Abacn force-pushed the revert-31818-excludestringset branch from 4f83c8c to 03b8b00 Compare July 26, 2024 20:50
@Abacn Abacn force-pushed the revert-31818-excludestringset branch from 03b8b00 to 03d1008 Compare July 26, 2024 20:52
@github-actions github-actions bot removed the build label Jul 26, 2024
@Abacn Abacn changed the title Revert "Exclude StringSet tests from portable runners and Dataflow LegacyRunner" Fix StringSet tests on portable runners Jul 26, 2024
@Abacn
Copy link
Contributor Author

Abacn commented Jul 26, 2024 •
edited
Loading

Ready for review. Latest commit reverted trigger file change. See test results of f302fb1

R: @rohitsinha54

@github-actions
Copy link
Contributor

github-actions bot commented Jul 26, 2024

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @kennknowles for label java.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@Abacn
Copy link
Contributor Author

Abacn commented Jul 29, 2024

R: @robertwb @rohitsinha54

@github-actions
Copy link
Contributor

github-actions bot commented Jul 29, 2024

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

@Abacn Abacn merged commit eec2068 into master Jul 31, 2024
@Abacn Abacn deleted the revert-31818-excludestringset branch July 31, 2024 19:16
reeba212 pushed a commit to reeba212/beam that referenced this pull request Dec 4, 2024
@Abacn @reeba212
Fix StringSet tests on portable runners (apache#31999)
29246d1 
* Revert "Exclude StringSet tests from portable runners and Dataflow LegacyRunn…"

This reverts commit b129433.

* Fix MetricsTest for StringSet metrics on portable runners

* trigger tests

* Re-exclude tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@rohitsinha54 rohitsinha54 rohitsinha54 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

dataflow flink java Next Action: Reviewers portability runners samza spark

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Abacn @rohitsinha54